Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Check if events must be on main #2253

Merged
merged 17 commits into from
Aug 9, 2024
Merged

Check if events must be on main #2253

merged 17 commits into from
Aug 9, 2024

Conversation

zeitschlag
Copy link
Collaborator

More minor refactorings. I checked every notification posted in events.swift if posting on main was necessary and it turned out: Most of the stuff gets already handled on main (except one or two, I fixed that).

Closes #2242.

Base automatically changed from event-refactoring-progress-alerts to main August 9, 2024 06:43
@zeitschlag
Don't send .incomingMessage on main (#2242)
6d482da 
Events are already handled on main (where necessary) on the receiving side
@zeitschlag
Don't send IncomingMessageOnAnyAccount on main (#2242)
b90d080
@zeitschlag
Don't send unused notification on main (#2242)
e9a69a5
@zeitschlag
Check for Event.contactsChanged if main is necessary (#2242)
d06065b 
It's not
@zeitschlag
Fix build 🤦 (#2242)
8b4715f
@zeitschlag
Don't post Event.connectivityChanged on main (#2242)
03823e3
@zeitschlag
There's no need to post Realtime-notifications on main (#2242)
86e6e54
@zeitschlag
There's no need to post chatModified-notifications on main (#2242)
a5d21d5
@zeitschlag
There's no need to post messagesNoticed-notifications on main (#2242)
c7f45c7 
as stuff gets handled on Main nevertheless
@zeitschlag
There's no need to post messageReadDeliveredFailedReaction-notificati…
50a46bb 
…ons on main (#2242)

as, you guessed it, stuff already gets handled on main
@zeitschlag
There's no need to post Event.messagesChanged-notifications on main (#…
3733cf8 
…2242)
@zeitschlag
There's no need to post Event.importExportProgress-notifications on m…
07d028d 
…ain (#2242)

if you can handle that stuff on main
@zeitschlag
There's no need to post Event.configurationProgress-notifications on …
ac3fe2b 
…main (#2242)
@zeitschlag
Remove dead code
9172e1b 
"this method is unnecessary and shouldn't be used"
@zeitschlag zeitschlag marked this pull request as ready for review August 9, 2024 07:05
@zeitschlag zeitschlag requested a review from r10s August 9, 2024 07:05
@zeitschlag zeitschlag self-assigned this Aug 9, 2024
@zeitschlag zeitschlag added this to the 1.46.9 milestone Aug 9, 2024
r10s
r10s approved these changes Aug 9, 2024
View reviewed changes
Copy link
Member

@r10s r10s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice! code lgtm, some minors.

i also did test some standard things and double-checked some "more important" code flows - i did not run into issues yet

great job!

DcCore/DcCore/DC/events.swift Outdated Show resolved Hide resolved
DcCore/DcCore/DC/events.swift Outdated Show resolved Hide resolved
DcCore/DcCore/DC/events.swift Outdated Show resolved Hide resolved
DcCore/DcCore/DC/events.swift Outdated Show resolved Hide resolved
deltachat-ios/Controller/AccountSetup/Instand Onboarding/InstantOnboardingViewController.swift Show resolved Hide resolved
deltachat-ios/Controller/Settings/SettingsViewController.swift Show resolved Hide resolved
zeitschlag and others added 3 commits August 9, 2024 14:49
@zeitschlag @r10s
just normalising whitespace
43fa130 
Co-authored-by: bjoern <[email protected]>
@zeitschlag @r10s
Apply suggestions from code review
4b2a416 
Just normalising whitespace, thanks @r10s!

Co-authored-by: bjoern <[email protected]>
@zeitschlag @r10s
Apply suggestions from code review
b3896b5 
Co-authored-by: bjoern <[email protected]>
@zeitschlag zeitschlag merged commit a66fa75 into main Aug 9, 2024
1 check passed
@zeitschlag zeitschlag deleted the 2242-events-on-main branch August 9, 2024 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@r10s r10s r10s approved these changes

Assignees

@zeitschlag zeitschlag

Labels
None yet
Projects
None yet
Milestone
1.46.8
Development

Successfully merging this pull request may close these issues.

Doublecheck if Events must be sent on Main
2 participants
@zeitschlag @r10s